Updates to ThumbnailLoader and ThumbnailService #5827

rgozim · 2018-08-21T14:42:44Z

What this PR does

Updates thumbnail loading of the Insight client to try convey the state of an images import status to a user.

Code has been added to Insight to check if an image to be displayed has pyramids. If it does, a check is performed to determine if an image has completed it's import process. Please note, this PR assumes that the final, and slowest, part of an image import is the pyramid generation.

Server code has been updated to only return a thumbnail if a thumbnail is available. Otherwise, nothing is returned and it becomes the clients responsibility to display a loading symbol.

Testing this PR

Setup a local OMERO.server, and import some images greater than 2K width/height (squig/ome/data_repo/test_images_good/png)

To do: add testing steps

Related reading

Trello card

Changes

Adds an extra public method to the ThumbnailStore / ThumbnailBean API
Changes logic of ThumbnailLoader in Insight.

mtbc · 2018-08-21T14:44:35Z

components/insight/SRC/org/openmicroscopy/shoola/env/data/model/ThumbnailData.java

@@ -21,13 +21,14 @@

 package org.openmicroscopy.shoola.env.data.model;

-import java.awt.Graphics2D;
+import java.awt.*;


no wildcard imports

ThumbnailLoader calls new API, checks if image has pyramids and can determine what state a thumbnail is in if it is missing ThumbnailBean has additional simple load method that only returns a thumbnail if is available, otherwise returns an empty array

mtbc · 2018-08-22T08:21:58Z

components/server/src/ome/services/ThumbnailBean.java

@@ -1423,4 +1488,4 @@ public void setDiskSpaceChecking(boolean diskSpaceChecking) {
 }
 }

-}
+}


Can just leave this EOL here.

Add empty implementation to Destroyable

dominikl · 2018-08-22T09:45:02Z

Testing with a local build. Can't get the thumbnails in Insight:

java.lang.Exception: Abnormal termination due to an uncaught exception.
java.lang.NullPointerException: No images.
	at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.<init>(ThumbnailLoader.java:303)
...

Will do some investigation, maybe I can find out why...

dominikl · 2018-08-22T09:53:07Z

components/insight/SRC/org/openmicroscopy/shoola/env/data/views/calls/ThumbnailLoader.java

- if (maxWidth <= 0)
+ public ThumbnailLoader(SecurityContext ctx, Collection<DataObject> imgs,
+ int maxWidth, int maxHeight, Collection<Long> userIDs) {
+ if (images == null) {


That's the NPE... Typo, has to be imgs.

Concur:

Abnormal termination due to an uncaught exception. java.lang.NullPointerException: No images. at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.<init>(ThumbnailLoader.java:303) at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.<init>(ThumbnailLoader.java:330)

dominikl · 2018-08-22T10:00:53Z

Directly after the import of three images 2k, 4k, and 8k, a grey thumbnail is shown for the pyramid ones:

After waiting a bit and refreshing an error is shown:

Blitz.log:

2018-08-22 10:55:17,794 WARN  [        ome.services.util.ServiceHandler] (.Server-13) Unknown exception thrown.

java.lang.NullPointerException: null
	at ome.services.ThumbnailCtx.dirtyMetadata(ThumbnailCtx.java:537) ~[server.jar:na]
	at ome.services.ThumbnailCtx.isThumbnailCached(ThumbnailCtx.java:578) ~[server.jar:na]
	at ome.services.ThumbnailBean.retrieveThumbnail(ThumbnailBean.java:1207) ~[server.jar:na]
	at ome.services.ThumbnailBean.getThumbnailWithoutDefault(ThumbnailBean.java:1104) ~[server.jar:na]
...

joshmoore

Along with the s/thumbnailMetadata/thumbMetaData/ I'd also suggest you force push away all the whitespace changes.

joshmoore · 2018-08-22T11:00:16Z

components/server/src/ome/services/ThumbnailBean.java

 StopWatch s1 = new Slf4JStopWatch("omero._createThumbnail");
- if (thumbnailMetadata == null) {
+ if (thumbMetaData == null) {


Why did this variable get changed?

It's now a local variable for the method. A new method that passes in thumbnailMetadata as a parameter was added.

…n DataBrowserModel

joshmoore · 2018-08-24T13:01:31Z

components/server/src/ome/services/ThumbnailBean.java

+ }
+
+ byte[] value = retrieveThumbnail(thumbnailMetadata);
+ // I don't really know why this is here, no iquery calls being that I can see...


This has to do with the Hibernate/transaction handling of this service since it's stateful.

mtbc · 2018-08-28T14:20:59Z

components/server/src/ome/services/ThumbnailBean.java

+ }
+ }
+
+ /**


This comment wasn't Javadoc. If you want it to be then best to remove the "non-Javadoc" and fix the @see and add a bit more.

…k still is returned in some cases

… allocation

dominikl · 2018-08-31T13:24:02Z

components/insight/SRC/org/openmicroscopy/shoola/env/data/views/calls/ThumbnailLoader.java

+
+ private ThumbnailStorePrx getThumbnailStore(PixelsData pxd) throws DSAccessException,
+ DSOutOfServiceException, ServerError {
+ // System.out.println(image.getId());


Could just be removed.

dominikl · 2018-08-31T13:27:18Z

Looks good. There's just a commented out System.out.println which could be removed. Apart from that, good to merge from my point of view.

joshmoore

Two javadoc fixes needed

joshmoore · 2018-09-03T07:50:57Z

components/insight/SRC/org/openmicroscopy/shoola/util/VersionCompare.java

+ * @return The result is a negative integer if str1 is _numerically_ less than str2.
+ * The result is a positive integer if str1 is _numerically_ greater than str2.
+ * The result is zero if the strings are _numerically_ equal.
+ * @note It does not work if "1.10" is supposed to be equal to "1.10.0".


This is failing javadoc: https://ci.openmicroscopy.org/job/OMERO-DEV-merge-build/ICE=3.6,jdk=8_LATEST,label=octopus/1064/warnings42Result/source.6931003519486980497/#8

components/server/src/ome/services/ThumbnailBean.java

joshmoore · 2018-09-03T07:59:19Z

components/insight/SRC/org/openmicroscopy/shoola/env/data/views/calls/ThumbnailLoader.java

+ store.setRenderingDefId(rndDefId);
+ }
+
+ if (VersionCompare.compare(context.getGateway().getServerVersion(), VERSION_THUMBNAIL_NO_DEFAULT) >= 0) {


@dominikl and @jburel will know better, but I assume there is a location that a boolean server_5_4_8_or_later could be saved so as to not need this in the loop.

It's not a big issue as the gateway holds the server version as string, it doesn't request it over and over again. But if you want to keep it in Insight directly, you could add a LookupNames.SERVER_VERSION (or LookupNames.5_4_8_OR_LATER or something like that) and 'bind' it to the Registry when Insight connects to the server somewhere around here: https://github.com/openmicroscopy/openmicroscopy/blob/ffa5419fb669871f959973f68b2c3fa0e4e2b576/components/insight/SRC/org/openmicroscopy/shoola/env/data/DataServicesFactory.java#L575 (like the 'SESSION_KEY' field is bound to the Registry 3 lines above).

Here's where Gateway obtains it's server version during connection/session creation https://github.com/openmicroscopy/openmicroscopy/blob/040837bf61ce493d0a742a3fe871acc60847407d/components/blitz/src/omero/gateway/Gateway.java#L1066

I'll leave it to you guys to work out what's most "Insight-esque"; my point is mostly about whether or not the albeit slight overhead of a redudant check for each and every thumbnail is a concern.

…sion of server connected to. Cleaned up javadoc

dominikl · 2018-09-03T10:42:25Z

Looks good: For >= 5.4.8 the "Loading" thumbnail is displayed, for < 5.4.8 it's the clock. I tried to move the check to the login/connect method and add 'LookupName' to do it more the "Insight-esque" way. Don't know if you want to include it or have a different idea, rgozim#3 .

dominikl · 2018-09-03T11:37:01Z

components/insight/SRC/org/openmicroscopy/shoola/env/data/views/calls/ThumbnailLoader.java

+ int maxWidth = Integer.parseInt(getConfigService()
+ .getConfigValue("omero.pixeldata.max_plane_width"));
+ int maxHeight = Integer.parseInt(getConfigService()
+ .getConfigValue("omero.pixeldata.max_plane_height"));


I think these two properties should go into the connect method as well and get stored as 'LookupName' so that they are only requested once.

joshmoore · 2018-09-03T14:07:04Z

Thanks, guys. Considering the significant greenness of https://ci.openmicroscopy.org/job/OMERO-DEV-merge-daily/1015/ I'm going to merge this for an initial build. A combination of @dominikl's PR as well as handling of max_height etc. can come as a follow-up PR.

joshmoore · 2018-09-03T14:48:50Z

components/server/src/ome/services/ThumbnailBean.java

@@ -1227,11 +1317,11 @@ protected void actOnOneGroup(Set<Long> pixelsIds) {
 thumbnailMetadata = local;
 }

- BufferedImage image = createScaledImage(theZ, theT);
+ BufferedImage image = inProgress? null : createScaledImage(theZ, theT);


@rgozim : can you explain this change?

Ok, now a couple days late for our build so I'm merging for rc1. I think this change still needs explanation since (if I'm reading the diff correctly) it's the only change in logic outside of your new methods. What change in behavior did you need that led to it? How does it behave for bulk and non-bulk methods? Etc.

joshmoore · 2018-09-05T09:43:43Z

@will-moore points out that iviewer makes use of the single thumbnail loading method in omero.gateway so we will need to also test its behavior (already installed on the testing server).

joshmoore · 2018-09-06T10:15:36Z

components/insight/SRC/org/openmicroscopy/shoola/env/data/views/calls/ThumbnailLoader.java

+ // System.out.println(image.getId());
+ ThumbnailStorePrx store = service.createThumbnailStore(ctx);
+ if (!store.setPixelsId(pxd.getId())) {
+ store.resetDefaults();


This line is no longer covered in a try/catch block as it was previously. (cF. https://github.com/openmicroscopy/openmicroscopy/pull/5827/files?utf8=%E2%9C%93&diff=unified&w=1#diff-cc769d8303d8c7e9b3f1a015e3d7abfdL133)

This leads to:

serverExceptionClass = "ome.conditions.ApiUsageException" message = "Unable to reset rendering settings in a read-only group for Pixels set id:149552" at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:423) at java.lang.Class.newInstance(Class.java:442) at IceInternal.BasicStream.createUserException(BasicStream.java:2779) at IceInternal.BasicStream.access$300(BasicStream.java:14) at IceInternal.BasicStream$EncapsDecoder10.throwException(BasicStream.java:3298) at IceInternal.BasicStream.throwException(BasicStream.java:2291) at IceInternal.OutgoingAsync.throwUserException(OutgoingAsync.java:399) at omero.api.ThumbnailStorePrxHelper.end_resetDefaults(ThumbnailStorePrxHelper.java:3614) at omero.api.ThumbnailStorePrxHelper.resetDefaults(ThumbnailStorePrxHelper.java:3501) at omero.api.ThumbnailStorePrxHelper.resetDefaults(ThumbnailStorePrxHelper.java:3488) at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.getThumbnailStore(ThumbnailLoader.java:301) at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.access$000(ThumbnailLoader.java:70) at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader$1.doCall(ThumbnailLoader.java:223)

mtbc reviewed Aug 21, 2018

View reviewed changes

rgozim force-pushed the update_thumbnail_loading branch from 4250654 to e7ede6b Compare August 21, 2018 14:46

rgozim force-pushed the update_thumbnail_loading branch from e7ede6b to 78727d9 Compare August 21, 2018 14:47

mtbc reviewed Aug 22, 2018

View reviewed changes

dominikl and others added 2 commits August 22, 2018 10:19

Add empty implementation to Destroyable

2627ed7

Merge pull request #2 from dominikl/update_thumbnail_loading_dom

5b9a770

Add empty implementation to Destroyable

dominikl reviewed Aug 22, 2018

View reviewed changes

joshmoore added the exclude label Aug 22, 2018

joshmoore requested changes Aug 22, 2018

View reviewed changes

rgozim added 4 commits August 22, 2018 15:35

Fixed null images error

194a65b

Fixed thumbnail loading bug

28f3d0a

Not sure what's going, thumbnails load, but not in all cases

e12a6a9

Fixed thumbnail loading issue and cleaned up thumbnail loading code i…

1452941

…n DataBrowserModel

joshmoore reviewed Aug 24, 2018

View reviewed changes

joshmoore removed the exclude label Aug 28, 2018

mtbc reviewed Aug 28, 2018

View reviewed changes

rgozim added 4 commits August 28, 2018 17:36

Cleaned up some code in insight, however discovered that default cloc…

fe5598e

…k still is returned in some cases

Added local inpogress boolean to method

4117185

Fixed a javadoc comment on _createThumbnail method

9f57aa6

Restored DataBrowserModel.createThumbnailsLoader due to better thread…

fbc04e5

… allocation

dominikl reviewed Aug 31, 2018

View reviewed changes

joshmoore approved these changes Aug 31, 2018

View reviewed changes

rgozim added 2 commits August 31, 2018 14:31

Removed commented out code

93e5e19

Added ability to support older versions of omero.server less than 5.4.8

2e576bc

joshmoore requested changes Sep 3, 2018

View reviewed changes

joshmoore reviewed Sep 3, 2018

View reviewed changes

rgozim force-pushed the update_thumbnail_loading branch from f54c3b5 to 466e94f Compare September 3, 2018 08:49

Switched to using Gateway.getServerVersion function for obtaining ver…

9c4b34e

…sion of server connected to. Cleaned up javadoc

rgozim force-pushed the update_thumbnail_loading branch from 466e94f to 9c4b34e Compare September 3, 2018 09:03

dominikl reviewed Sep 3, 2018

View reviewed changes

joshmoore added this to the 5.4.8 milestone Sep 3, 2018

joshmoore approved these changes Sep 3, 2018

View reviewed changes

joshmoore reviewed Sep 3, 2018

View reviewed changes

joshmoore merged commit d1b488e into ome:develop Sep 4, 2018

joshmoore reviewed Sep 6, 2018

View reviewed changes

dominikl mentioned this pull request Sep 10, 2018

Add (temp) LookupName for checking server >= 5.4.8 rgozim/openmicroscopy#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to ThumbnailLoader and ThumbnailService #5827

Updates to ThumbnailLoader and ThumbnailService #5827

rgozim commented Aug 21, 2018 •

edited

Loading

mtbc Aug 21, 2018

mtbc Aug 22, 2018

dominikl commented Aug 22, 2018 •

edited

Loading

dominikl Aug 22, 2018

joshmoore Aug 22, 2018 •

edited

Loading

dominikl commented Aug 22, 2018 •

edited

Loading

joshmoore left a comment

joshmoore Aug 22, 2018

rgozim Aug 23, 2018

joshmoore Aug 24, 2018

mtbc Aug 28, 2018 •

edited

Loading

dominikl Aug 31, 2018

dominikl commented Aug 31, 2018

joshmoore left a comment

joshmoore Sep 3, 2018

joshmoore Sep 3, 2018

dominikl Sep 3, 2018 •

edited

Loading

rgozim Sep 3, 2018 •

edited

Loading

joshmoore Sep 3, 2018

dominikl commented Sep 3, 2018

dominikl Sep 3, 2018

joshmoore commented Sep 3, 2018

joshmoore Sep 3, 2018

joshmoore Sep 4, 2018

joshmoore commented Sep 5, 2018

joshmoore Sep 6, 2018

  }
  }
-}
+}

+ }
+ }
+ /**

Updates to ThumbnailLoader and ThumbnailService #5827

Updates to ThumbnailLoader and ThumbnailService #5827

Conversation

rgozim commented Aug 21, 2018 • edited Loading

What this PR does

Testing this PR

Related reading

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikl commented Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

joshmoore Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

dominikl commented Aug 22, 2018 • edited Loading

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtbc Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikl commented Aug 31, 2018

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikl Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

rgozim Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikl commented Sep 3, 2018

Choose a reason for hiding this comment

joshmoore commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore commented Sep 5, 2018

Choose a reason for hiding this comment

rgozim commented Aug 21, 2018 •

edited

Loading

dominikl commented Aug 22, 2018 •

edited

Loading

joshmoore Aug 22, 2018 •

edited

Loading

dominikl commented Aug 22, 2018 •

edited

Loading

mtbc Aug 28, 2018 •

edited

Loading

dominikl Sep 3, 2018 •

edited

Loading

rgozim Sep 3, 2018 •

edited

Loading